Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add image constructor from compatible view #520

Merged
merged 3 commits into from
Oct 11, 2020

Conversation

sdebionne
Copy link
Contributor

@sdebionne sdebionne commented Oct 2, 2020

Description

For convenience, this PR introduces a new image constructor to "materialize" (in the SQL sense) an image_view.

Tasklist

  • Add test case(s)
  • Ensure all CI builds pass
  • Review and approve

@sdebionne sdebionne marked this pull request as ready for review October 5, 2020 08:00
Copy link
Member

@lpranam lpranam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@mloskot mloskot added the cat/feature New feature or functionality label Oct 6, 2020
@mloskot
Copy link
Member

mloskot commented Oct 6, 2020

Although this looks good to me too, I'm having issue about the terminology vs the code here.

The subject of this PR says "from compatible view" brief look at the code immediately raised me a question:

  • Why not to enable_if with views_are_compatible (de-facto pixels_are_compatible)?

In GIL, definition of view/pixel compatibility is based on color space compatibilty and the latter says:

struct color_spaces_are_compatible : std::is_same<CS1, CS2> {};

which obviously is not equivalent of what this PR proposes.

In GIL, we may want to distinguish view/pixel compatibility from view/pixel convertibility.

Would you agree that the "from compatible view" should read "from convertible view"?

@sdebionne
Copy link
Contributor Author

Why not to enable_if with views_are_compatible (de-facto pixels_are_compatible)?

Right, I forgot about views_are_compatible and that would be better to use GIL traits to check compatibility between the target image and the source view.

How about typename std::enable_if<views_are_compatible<image_view<Loc>, view_t >::value, int>::type = 0>?

Is std::convertible<P1, P2> (P1 can be converted to P2 using implicit conversions) different from pixels_are_compatible< P1, P2> (Compatible pixels can be assigned and copy constructed from one another)?

Note that allocate_and_copy uses uninitialized_copy_pixels internally, not copy_and_convert_pixels. So is it compatibility or convertibility?

I am confused. Maybe we need some examples to clarify "convertible" vs "compatible"? My guess is

gray8 -> gray16 : convertible?
rgb8 -> bgr8 : compatible?

@mloskot
Copy link
Member

mloskot commented Oct 9, 2020

@sdebionne Sorry for delay.

How about typename std::enable_if<views_are_compatible<image_view<Loc>, view_t >::value, int>::type = 0>?

Well, you know it best if this would be suitable for your intention in this PR.

Is std::convertible<P1, P2> different from pixels_are_compatible<P1, P2>?

Yes, to me it is different.

The pixels_are_compatible is stricter, in terms of std:is_same for pixel components.
For non-compatible, but convertible, an explicit conversion via e.g. copy_and_convert_pixels is necessary,

I am confused.

Me too :-)

Maybe we need some examples to clarify "convertible" vs "compatible"?

Yes we do!

I guess, we could also start developing some more systematic system of type traits in GIL:

gil::is_color_convertible<T1, T2>
gil::is_color_compatible<T1, T2>

where T1 and T2 stand either for image, view, pixel, color space and even down to channel type.
Leaving the existing views_are_compatible and pixels_are_compatible alone, for now.

What do you think?

My guess is

gray8 -> gray16 : convertible?

That is how I understand it.

rgb8 -> bgr8 : compatible?

That is how I understand it, thanks to semantic kth-dereferencing of channels.

@sdebionne
Copy link
Contributor Author

Thanks for the clarification! I'll go for compatibility with this PR and use the more strict pixels_are_compatible.

@mloskot
Copy link
Member

mloskot commented Oct 9, 2020

Cool. What do you think about the traits idea? Is this anything you'd see useful, a good direction, in future?

@mloskot mloskot added this to the Boost 1.75+ milestone Oct 11, 2020
@mloskot mloskot changed the title Add an image ctor from compatible view Add image constructor from compatible view Oct 11, 2020
@mloskot mloskot merged commit f374a67 into boostorg:develop Oct 11, 2020
@mloskot
Copy link
Member

mloskot commented Oct 11, 2020

Thanks @sdebionne

@sdebionne sdebionne deleted the add-image-ctor-from-view branch October 12, 2020 12:30
@mloskot mloskot modified the milestones: Boost 1.75, Boost 1.76+ Nov 10, 2020
@mloskot mloskot mentioned this pull request May 12, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/feature New feature or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants